Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(labelling): hotfix for issues related to deploy erroring due to nested yaml field overriding in StatefulSet w/ PVC #7011

Merged
merged 1 commit into from
Jan 13, 2022
Merged

fix(labelling): hotfix for issues related to deploy erroring due to nested yaml field overriding in StatefulSet w/ PVC #7011

merged 1 commit into from
Jan 13, 2022
+20 −13

Conversation

aaron-prindle
Copy link
Contributor

@aaron-prindle aaron-prindle commented Jan 13, 2022 •
edited
Loading

Hotfix for an issue that is being seen in which when skaffold attempts to redeploy manifests with nested objects and "immutable" fields, skaffold fails as it will attempt to override the label/image when doing so will result in a deployment error.

Minimal repro:

Skaffold.yaml

apiVersion: skaffold/v2beta12
kind: Config
deploy:
  kubectl:
    manifests:
      - k8-*

k8-pod.yaml:

apiVersion: apps/v1
kind: StatefulSet
metadata:
  name: web
spec:
  selector:
    matchLabels:
      app: echoserver # has to match .spec.template.metadata.labels
  serviceName: "echoserver"
  replicas: 3 # by default is 1
  template:
    metadata:
      labels:
        app: echoserver 
    spec:
      terminationGracePeriodSeconds: 10
      containers:
      - image: k8s.gcr.io/echoserver:1.4
        name: echoserver
        volumeMounts:
        - name: www
          mountPath: /usr/share/nginx/html
  volumeClaimTemplates:
  - metadata:
      name: www
    spec:
      accessModes: [ "ReadWriteOnce" ]
      resources:
        requests:
          storage: 1Gi

run "skaffold deploy" twice w/o this PR fails with:

aprindle@aprindle ~/repro-6416 $ skaffold deploy
Tags used in deployment:
Starting deploy...
 - The StatefulSet "web" is invalid: spec: Forbidden: updates to statefulset spec for fields other than 'replicas', 'template', 'updateStrategy' and 'minReadySeconds' are forbidden
kubectl apply: exit status 1[

works w/ this change.

A more extensible/robust approach to fixing this issue is in progress as well that will implement something similar to the proposed idea from - #6236 (found here - https://github.com/GoogleContainerTools/skaffold/blob/main/docs/design_proposals/configurable-transformable-allowlist.md)

@aaron-prindle aaron-prindle requested a review from a team as a code owner January 13, 2022 01:40
@aaron-prindle aaron-prindle requested review from briandealwis and removed request for gsquared94 January 13, 2022 01:42
@aaron-prindle aaron-prindle force-pushed the fix-6416-update-manifest-allowlist-functionality-v2 branch from 0677db1 to d3ddeee Compare January 13, 2022 01:43
@codecov
Copy link

codecov bot commented Jan 13, 2022 •
edited
Loading

Codecov Report

Merging #7011 (31df8c8) into main (290280e) will decrease coverage by 1.71%.
The diff coverage is 56.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7011      +/-   ##
==========================================
- Coverage   70.48%   68.77%   -1.72%     
==========================================
  Files         515      554      +39     
  Lines       23150    25695    +2545     
==========================================
+ Hits        16317    17671    +1354     
- Misses       5776     6822    +1046     
- Partials     1057     1202     +145
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/deploy.go 52.00% <ø> (-1.85%) ⬇️
cmd/skaffold/app/cmd/dev.go 84.61% <0.00%> (ø)
cmd/skaffold/app/cmd/render.go 36.66% <0.00%> (-4.72%) ⬇️
cmd/skaffold/skaffold.go 0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/inspect_tests.go 62.50% <14.28%> (-1.14%) ⬇️
cmd/skaffold/app/cmd/lsp.go 28.12% <28.12%> (ø)
cmd/skaffold/app/cmd/fix.go 68.85% <40.00%> (-7.62%) ⬇️
cmd/skaffold/app/cmd/lint.go 42.85% <42.85%> (ø)
cmd/skaffold/app/cmd/find_configs.go 48.88% <50.00%> (+0.24%) ⬆️
cmd/skaffold/app/skaffold.go 76.19% <70.00%> (-8.43%) ⬇️
... and 181 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8cd8b69...31df8c8. Read the comment docs.

@aaron-prindle aaron-prindle force-pushed the fix-6416-update-manifest-allowlist-functionality-v2 branch from d3ddeee to e758885 Compare January 13, 2022 02:30
@aaron-prindle aaron-prindle force-pushed the fix-6416-update-manifest-allowlist-functionality-v2 branch 2 times, most recently from 9115b39 to 9f726e9 Compare January 13, 2022 02:38
@aaron-prindle aaron-prindle changed the title fix(labelling): hotfix for issues related to deploy erroring due to nested yaml overriding in StatefulSet w/ PVC fix(labelling): hotfix for issues related to deploy erroring due to nested yaml field overriding in StatefulSet w/ PVC Jan 13, 2022
@aaron-prindle aaron-prindle force-pushed the fix-6416-update-manifest-allowlist-functionality-v2 branch 3 times, most recently from 905e64e to e5f3f34 Compare January 13, 2022 05:54
briandealwis
briandealwis approved these changes Jan 13, 2022
View reviewed changes
Copy link
Member

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one caveat/question: is jsonpath a valid JSONPath string? If not, could you rename jsonpath to navpath or something similar to avoid any confusion that this the contents is actually a JSONPath string?

pkg/skaffold/kubernetes/manifest/visitor.go Outdated Show resolved Hide resolved
pkg/skaffold/kubernetes/manifest/visitor.go Outdated Show resolved Hide resolved
@aaron-prindle aaron-prindle force-pushed the fix-6416-update-manifest-allowlist-functionality-v2 branch from e5f3f34 to a3577fb Compare January 13, 2022 19:12
@aaron-prindle
Copy link
Contributor Author

LGTM with one caveat/question: is jsonpath a valid JSONPath string? If not, could you rename jsonpath to navpath or something similar to avoid any confusion that this the contents is actually a JSONPath string?

Good idea. No I don't believe it is a valid JSONPath string as we cannot get the array index values which I believe is part of that spec. I have renamed the field "navpath"

@aaron-prindle aaron-prindle force-pushed the fix-6416-update-manifest-allowlist-functionality-v2 branch from a3577fb to 23335e8 Compare January 13, 2022 19:18
@aaron-prindle
fix(labelling): hotfix for issues related to deploy erroring due to n…
31df8c8 
…ested yaml field overriding in StatefulSet w/ PVC
@aaron-prindle aaron-prindle force-pushed the fix-6416-update-manifest-allowlist-functionality-v2 branch from 23335e8 to 31df8c8 Compare January 13, 2022 19:46
@aaron-prindle aaron-prindle merged commit f2c7990 into GoogleContainerTools:main Jan 13, 2022
MarlonGamez pushed a commit that referenced this pull request Jan 13, 2022
@aaron-prindle @MarlonGamez
fix(labelling): hotfix for issues related to deploy erroring due to n…
8015cc2 
…ested yaml field overriding in StatefulSet w/ PVC (#7011)
@renovate renovate bot mentioned this pull request Jan 14, 2022
Merged
1 task
@aaron-prindle aaron-prindle mentioned this pull request Feb 22, 2022
Merged
@timebertt timebertt mentioned this pull request Jun 7, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@briandealwis briandealwis briandealwis approved these changes

@tejal29 tejal29 Awaiting requested review from tejal29

@nkubala nkubala Awaiting requested review from nkubala

Assignees
No one assigned
Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aaron-prindle @briandealwis